Fix: add crontab to acme.sh install dependency checks#17
Fix: add crontab to acme.sh install dependency checks#17W0lfD wants to merge 2 commits intoPasarGuard:mainfrom
Conversation
WalkthroughThe changes enhance ACME dependency management by introducing OS-specific cron package selection, installing cron packages when crontab is unavailable, and validating that the acme binary exists and is executable after installation. Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pasarguard.sh`:
- Around line 337-342: When calling get_cron_package_name before
install_package, check whether it succeeded and returned a non-empty value
(cron_pkg) and handle the failure path; if get_cron_package_name fails or
cron_pkg is empty, log an error and exit (or return a non-zero status) instead
of calling install_package with an empty argument. Update the block around
cron_pkg, get_cron_package_name and install_package to validate cron_pkg (and
propagate errors from get_cron_package_name) so install_package is only invoked
with a valid package name.
- Around line 324-332: get_cron_package_name currently mixes single-bracket
tests (which don't support globbing) and omits openSUSE; change the conditional
to use double-bracket tests for all OS pattern matches (e.g., [[ "$OS" ==
"Fedora"* ]]) so the glob suffix works, and add openSUSE to the branch that
returns "cronie" so it matches install_package's supported distro list; update
the elif to include CentOS, AlmaLinux, Fedora, openSUSE, and Arch Linux using [[
... ]] pattern matching and keep the else returning failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| get_cron_package_name() { | ||
| if [[ "$OS" == "Ubuntu"* ]] || [[ "$OS" == "Debian"* ]]; then | ||
| echo "cron" | ||
| elif [[ "$OS" == "CentOS"* ]] || [[ "$OS" == "AlmaLinux"* ]] || [ "$OS" == "Fedora"* ] || [ "$OS" == "Arch Linux" ]; then | ||
| echo "cronie" | ||
| else | ||
| return 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
Glob pattern won't work with single brackets; also missing openSUSE support.
Line 327: [ "$OS" == "Fedora"* ] uses single brackets which don't support glob matching. This condition will never match Fedora variants. The static analysis tool (Shellcheck SC2081) also flags this.
Additionally, openSUSE is supported in install_package() (line 207-209) but is not handled here, which will cause get_cron_package_name to return failure on openSUSE systems.
🐛 Proposed fix
get_cron_package_name() {
if [[ "$OS" == "Ubuntu"* ]] || [[ "$OS" == "Debian"* ]]; then
echo "cron"
- elif [[ "$OS" == "CentOS"* ]] || [[ "$OS" == "AlmaLinux"* ]] || [ "$OS" == "Fedora"* ] || [ "$OS" == "Arch Linux" ]; then
+ elif [[ "$OS" == "CentOS"* ]] || [[ "$OS" == "AlmaLinux"* ]] || [[ "$OS" == "Fedora"* ]] || [[ "$OS" == "Arch Linux" ]]; then
echo "cronie"
+ elif [[ "$OS" == "openSUSE"* ]]; then
+ echo "cronie"
else
return 1
fi
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get_cron_package_name() { | |
| if [[ "$OS" == "Ubuntu"* ]] || [[ "$OS" == "Debian"* ]]; then | |
| echo "cron" | |
| elif [[ "$OS" == "CentOS"* ]] || [[ "$OS" == "AlmaLinux"* ]] || [ "$OS" == "Fedora"* ] || [ "$OS" == "Arch Linux" ]; then | |
| echo "cronie" | |
| else | |
| return 1 | |
| fi | |
| } | |
| get_cron_package_name() { | |
| if [[ "$OS" == "Ubuntu"* ]] || [[ "$OS" == "Debian"* ]]; then | |
| echo "cron" | |
| elif [[ "$OS" == "CentOS"* ]] || [[ "$OS" == "AlmaLinux"* ]] || [[ "$OS" == "Fedora"* ]] || [[ "$OS" == "Arch Linux" ]]; then | |
| echo "cronie" | |
| elif [[ "$OS" == "openSUSE"* ]]; then | |
| echo "cronie" | |
| else | |
| return 1 | |
| fi | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 327-327: [ .. ] can't match globs. Use [[ .. ]] or case statement.
(SC2081)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pasarguard.sh` around lines 324 - 332, get_cron_package_name currently mixes
single-bracket tests (which don't support globbing) and omits openSUSE; change
the conditional to use double-bracket tests for all OS pattern matches (e.g., [[
"$OS" == "Fedora"* ]]) so the glob suffix works, and add openSUSE to the branch
that returns "cronie" so it matches install_package's supported distro list;
update the elif to include CentOS, AlmaLinux, Fedora, openSUSE, and Arch Linux
using [[ ... ]] pattern matching and keep the else returning failure.
|
|
||
| local cron_pkg | ||
| if ! command -v crontab >/dev/null 2>&1; then | ||
| cron_pkg="$(get_cron_package_name)" | ||
| install_package "$cron_pkg" | ||
| fi |
There was a problem hiding this comment.
Missing error handling when get_cron_package_name fails.
If get_cron_package_name returns failure (e.g., on an unsupported OS), cron_pkg will be empty, and install_package "" will be called with an empty argument, potentially causing unexpected behavior or confusing error messages.
🛡️ Proposed fix to add error handling
local cron_pkg
if ! command -v crontab >/dev/null 2>&1; then
- cron_pkg="$(get_cron_package_name)"
- install_package "$cron_pkg"
+ if cron_pkg="$(get_cron_package_name)"; then
+ install_package "$cron_pkg"
+ else
+ colorized_echo yellow "Warning: Could not determine cron package for this OS. Please install crontab manually."
+ fi
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local cron_pkg | |
| if ! command -v crontab >/dev/null 2>&1; then | |
| cron_pkg="$(get_cron_package_name)" | |
| install_package "$cron_pkg" | |
| fi | |
| local cron_pkg | |
| if ! command -v crontab >/dev/null 2>&1; then | |
| if cron_pkg="$(get_cron_package_name)"; then | |
| install_package "$cron_pkg" | |
| else | |
| colorized_echo yellow "Warning: Could not determine cron package for this OS. Please install crontab manually." | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pasarguard.sh` around lines 337 - 342, When calling get_cron_package_name
before install_package, check whether it succeeded and returned a non-empty
value (cron_pkg) and handle the failure path; if get_cron_package_name fails or
cron_pkg is empty, log an error and exit (or return a non-zero status) instead
of calling install_package with an empty argument. Update the block around
cron_pkg, get_cron_package_name and install_package to validate cron_pkg (and
propagate errors from get_cron_package_name) so install_package is only invoked
with a valid package name.
1 participant
Summary by CodeRabbit